Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a markmap mind map page for scaling InnerSource #901

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

jeffabailey
Copy link
Contributor

@jeffabailey jeffabailey commented Oct 18, 2024

An output of an @InnerSourceCommons/ispo-working-group meeting: https://docs.google.com/document/d/12PFkoiaukmrMbnTPYKERzViKCULfClutXN4ydVPv3H0/edit

feat: add markmap partial
feat: add markmap shortcode
feat: add markmap friendly markmap content
fix: skip yourmax processing if no items defined

feat: add markmap shortcode
feat: add markmap friendly markmap content
fix: skip yourmax processing if no items defined
@dellagustin-sap
Copy link
Contributor

dellagustin-sap commented Oct 18, 2024

Hi @jeffabailey , I tested the PR locally and found two issues:

  1. Major - the /learn page is showing some "weird" stuff, screenshot below, it shows the markdown text of the mindmap under "Scaling InnerSource" and it shows also an additional section with the content of markmap_content.md, but slightly different.
  2. Minor - On the scaling page, the mindmap has a kind of a viewport, where it can be zoomed in and out, panned and scrolled, but without scrollbars or boarders - this is more a weird feature than a bug, just wanted to mention

I found a way to fix 1 and I'll send a PR on top of this PR with a fix.

image image

This commit fixes an issue on the `/learn` page introduced with
the mindmap for scaling.

Acording to my tests:
1. If a folder does not have a `_index.md` file, every file under
   the folder will generate a section under the `/learn` page. in
   This way, both files, `mindmap.md` and `markmap_content.md`
   were generating content. To fix this, I renamed `mindmap.md`
   to `_index.md`.
2. When a file does not mave a `description` property in the
   frontmatter block, it will use the content of the file as
   description. To fix this I added a `description` property to
   `mindmap.md` (now renamed to `_index.md`.

Related issues
- #902 (comment)
@dellagustin-sap
Copy link
Contributor

@jeffabailey , here is the proposed fix: #902

weight: 1
---

{{< markmap path="markmap_content.md" >}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeffabailey all the other files on the repo use a - as separator.
I suggest renaming markmap_content.md to markmap-content.md for consistency.

@dellagustin-sap
Copy link
Contributor

Also, it would be nice if we could make the links open in a new tab, but I think that would require some implementation.
Here are some refs:

by adding a transformer plugin with something like this, we may be able to do it:

<script>
  window.markmap = {
    autoLoader: {
      transformPlugins: [...markmap.builtInPlugins, { 
     name: 'target-blank', 
     transform(transformHooks) { 
       transformHooks.parser.tap((md) => { 
         md.renderer.renderAttrs = wrapFunction( 
           md.renderer.renderAttrs, 
           (renderAttrs, token) => { 
             let attrs = renderAttrs(token); 
             if (token.type === 'link_open') { 
               attrs += ' target="_blank"'; 
             } 
             return attrs; 
           }, 
         ); 
       }); 
       return {}; 
     }, 
   }],
    },
  };
</script>

I'll need to test this later.

@jeffabailey jeffabailey marked this pull request as ready for review October 19, 2024 20:21
@jeffabailey jeffabailey requested a review from a team as a code owner October 19, 2024 20:21
@dellagustin-sap
Copy link
Contributor

dellagustin-sap commented Oct 21, 2024

LGTM, but I'm not a maintainer.
A minor quirk, viewing on my 15" macbook display, the viewport of the mindmap does not fit the screen in 100% zoom, vertically.
image

@jeffabailey
Copy link
Contributor Author

jeffabailey commented Oct 21, 2024

Unless we compact the mindmap, I'm afraid that's an avoidable quirk.

However, assuming that all operating systems and browsers allow you to hold ctrl to pan the mindmap in any direction, we could put a quick instruction above the mindmap.

@dellagustin-sap
Copy link
Contributor

However, assuming that all operating systems and browsers allow you to hold ctrl to pan the mindmap in any direction, we could put a quick instruction above the mindmap.

I was thinking about that too, some instructions on top would be nice. I don't think we need to hold the merge for that though, that could be an increment with another PR, unless you want to include it already, up to you @jeffabailey .

One minor ask, can you please link to https://docs.google.com/document/d/12PFkoiaukmrMbnTPYKERzViKCULfClutXN4ydVPv3H0/edit on the description of the PR? Just for additional traceability on the origin of the mindmap.

@jeffabailey
Copy link
Contributor Author

Hello @InnerSourceCommons/website-maintainers do you have any concerns with merging this PR?

If no, please approve. :)

feat: start map from level 2
feat: add complete Miro content
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants